-
Notifications
You must be signed in to change notification settings - Fork 228
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added validation and customization function in ghost app #3403
Conversation
@bgrant0607 @yuwenma @justinsb Added some package specific validation and customization function. Since they are package specific, I wrote them in starlark. Please take a look and let me know what you think. |
Is that intended to add the labels in line? |
return | ||
for resource in ctx.resource_list["items"]: | ||
if resource["kind"] == "Ingress" and resource["spec"]["rules"][0]["host"] == placeHolderHostname: | ||
fail(errMsg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly, this should be appended to ResourceList.Results
but it's not quite supported yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack.
I like this approach! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! A minor comment about removing the in-line labels
Can you elaborate more on this. The labels are added by |
|
||
for env in container["env"]: | ||
if env["name"] == "GHOST_HOST": | ||
env["value"] = host |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OOC, is ApplyReplacements capable of copying the host value from the Ingress resource to this environment variable in the associative list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will check.
Let's say if ApplyReplacements supports that, then, are you thinking of treating the value of host in ingress
resource to be source of truth ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the docs says it is possible to target value in a sequence node, so will give it a try.
errMsg = "Blog's hostname must be specified in fn/update-blog-host.yaml" | ||
placeHolderHostname = "example.com" | ||
for resource in resources: | ||
# this is an abstract package, so ignore host name validation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably should have a first-class way to query this in our sdks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, validation functions needs this bit, so a helper in SDK will help.
/cc @yuwenma
Using ApplyReplacements instead of Starlark for propagating the values. Also improved the validation function. So good enough for merging. Also I will be moving the package-examples to kpt-samples repo after tagging this version. |
* using apply-replacements to propagate the values.
This PR does the following:
blog host
blog host
kpt fn render